Skip to content

docs: document the command boundary and spec its public surface - #28

Merged
ausimian merged 8 commits into
release/1.0.0from
issue/11-docs
Aug 23, 2026
Merged

docs: document the command boundary and spec its public surface#28
ausimian merged 8 commits into
release/1.0.0from
issue/11-docs

Conversation

@ausimian

Copy link
Copy Markdown
Owner

Refs #11. Targets release/1.0.0. Documentation and typespecs only; no behaviour changed.

@moduledoc was the mix new placeholder and nothing but customize/1 carried a @doc or @spec, so the published HexDocs reference page listed undocumented, un-spec'd functions.

The issue is stale in two ways

It lists generate/1, which castle#13 deleted along with the build.config path — not documented, not resurrected. And it predates Castle.customize/1, which castle#12 documented in full; that is left untouched and is the standard the rest was written to match.

What is published and what is not

These are not an Elixir library API, and the docs have to say so or a reader is surprised twice — by functions that print rather than return, and by ones that raise rather than answer. Castle is the command boundary.

  • make_releases/0@doc false, with a @spec. Its only caller is Forecastle's env.sh fragment invoking --eval "Castle.make_releases()" in the preboot VM, gated on releases/RELEASES being absent. There is no bin/castle subcommand, and by hand it either does nothing or does what the next start does anyway. Its contract is with a shell fragment in another project; the spec stays because a spec is the contract whether or not the page shows it.
  • install/2..5 — documented as the test seam they are. One @doc covers all arities of a defaulted clause, and ExDoc renders the signature with all four defaults visible, so silence would read as API. install/1 is the operator's form; the rest exist so a concurrency test can drive the public boundary, which AGENTS.md explains at length.
  • running/1 — documented, though it has no subcommand of its own. The bin/castle subcommands are exactly commit, install, releases, remove, unpack, upgradable; running/1's only shipped caller is the confirmation loop inside bin/castle install. It is documented anyway because it answers a read-only question and automation driving rpc directly needs precisely it — a different case from make_releases/0, which mutates bookkeeping once and is useless by hand.
  • The rest are documented naming the bin/castle command that reaches them, what they refuse, and what they leave behind.

Every public function carries a @spec, and they are all :: :ok (keyword() for customize/1) because that is the whole of what report!/1 returns: Enum.each/2 on success, a raise otherwise.

Seven claims the first pass got wrong

The first commit here documented the surface; a review of it found seven false or overstated claims, all fixed in the second. Worth listing, because this branch is nothing but assertions and there is no Dialyzer checking any of them:

  1. Castle.Error is not the only failure. Commands.installed/5 deliberately re-raises an exception, throw or exit out of install_release/1 once the marker is settled. Automation rescuing only Castle.Error misses a command that blew up. AGENTS.md carried the same unqualified claim and is corrected too.
  2. upgradable/0 had reintroduced a claim this repo corrected five times — that a synthesised record means releases/RELEASES was unreadable. It says what the refusal message says: the file was not one :release_handler accepted, no single property of the file is the test, and the path is qualified with RELDIR / {sasl, releases_dir} (castle#23).
  3. The install preflight was documented in the wrong order, and as four steps rather than five. Arming is also before install_release/1 and can also refuse — with the configuration already written, which is exactly what occupied/3 and stale/4 tell an operator. The boundary is install_release/1, not the first call into :release_handler: the release-record lookup is such a call.
  4. Rollback was overstated. After a restart transition the reboot the install asked for is already accounted for — the launcher consumes the marker pair and boots the target. It is the restarts after that one, before commit, which read start_erl.data and return. RELEASE.md repeated the unqualified version.
  5. Commit is not a no-op for an already-permanent release. OTP's promotion step is, but Commands.commit/5 materialises first since castle#14, so an explicit commit rewrites sys.config from current provider inputs and a failing provider now fails the command. Separately the argumentless form selects the release awaiting commitwhich_releases(:current) — not "the version running now".
  6. Three of five install arities had no spec, contradicting this branch's own claim that every public function carries one. Added, and verified with Code.Typespec.fetch_specs/1 rather than assumed — ExDoc renders only the widest arity's spec for a defaulted definition, so the page looked identical with two specs as with five and the gap was undetectable from mix docs.
  7. :unpacked does not mean "never installed". A failed or rolled-back continuation leaves a version in that state; it is now described as staged and not currently installed, including that case.

Also corrected, found while checking claims

Three internal notes left stale by castle#14's commit serialisation: materialise/3's @doc said the ERTS guard "is not redundant for Castle.commit/1, which still composes this in front of commit/3" — commit materialises inside its own serialised region now and there is no commit/3; two more commit/3 references in install/5's @doc; and a serialised/2 comment pointing at a nonexistent Castle.materialise/1.

AGENTS.md

The upgradable/0 bullet's "whether it belongs in the documented API surface is #11's to settle" is settled in place. The Known limitations bullet for the undocumented API is replaced by "Nothing checks the @specs" — no Dialyzer, so a spec or a refusal-claim that stops being true fails nothing, and both are read against Castle.Commands by hand. It also records that a per-arity spec gap is invisible to mix docs and must be counted against Castle.__info__(:functions).

Verification

mix precommit green — 154 tests, credo --strict clean. mix compile --warnings-as-errors --force clean under Elixir 1.20.3/OTP 28.4.1, which matters because this branch is nothing but specs. mix docs clean: two ex_doc warnings about Kernel.CLI being hidden (it is @moduledoc false, so it can be named but not linked) were fixed by naming elixir --rpc-eval instead, as Castle.Error's own moduledoc already did.

No tests added. The claims the docs make are already pinned — test/castle_test.exs asserts the raises, the silent successes and the printing; test/castle/erts_guard_test.exs pins that every gated command raises while upgradable/0 and releases/0 still answer, which is the moduledoc's closing paragraph. Asserting on documentation prose would be worse than nothing.

Out of scope, and one thing it turned up

castle#9 (README) and castle#8 (coverage) are untouched. Castle.Commands has the same per-arity spec gap across 18 arities; left alone deliberately, since it is @moduledoc false and no claim in the repo covers it.

Finding 5 also existed on the shell side — bin/castle --help said commit [VSN] "Defaults to the version that is running now", contradicting the code twenty lines above it. Fixed separately on Forecastle's release/1.0.0 in a684dae, along with a test whose name implied the same wrong thing.

ausimian and others added 2 commits August 23, 2026 23:44
The @moduledoc was still the placeholder `mix new` writes and none of
the commands carried a @doc or a @SPEC, so the published page said
nothing about the one thing a reader has to know before calling any of
it: these are not an Elixir API. Every function but `customize/1` is a
command entry point that `bin/castle` reaches over `rpc` - it prints its
report and returns a bare `:ok`, and it raises `Castle.Error` on failure
so that the calling VM exits non-zero. A reader who takes them for
ordinary functions is surprised by both halves, so the moduledoc says
what a command boundary is and every @doc names the `bin/castle` command
that reaches it. castle#11.

The distinction the issue asked to be settled, settled deliberately:

  * `make_releases/0` is @doc false. Its only caller is the launcher's
    `env.sh` fragment, in the preboot VM of a start whose deployment has
    no `RELEASES` yet, and by hand it either does nothing or does what
    the next start would do anyway. It keeps its @SPEC: the spec is the
    contract whether or not the function is published.

  * `install/2..5` is documented as what it is. One @doc covers every
    arity of a clause with defaults, so silence about the extra four
    would leave a test seam reading as an API.

  * Everything else is a command an operator invokes, and is documented
    with that framing rather than hidden - `upgradable/0` included,
    since the state it reports is invisible otherwise and a diagnostic
    nobody is told about is one nobody thinks to ask.

The specs say `:: :ok` and nothing more, because that is the whole of
what `report!/1` returns. `mix docs` is clean; its one complaint was a
reference to `Kernel.CLI`, which is hidden, so `Castle.Error`'s own
wording is used instead.

Three internal notes had gone stale and are corrected with it: two named
a `commit/3` that is now `commit/5`, and one claimed `commit` still
composes materialisation outside the serialised region, which castle#14
ended.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
Review of 01ccbf2 found seven inaccuracies, all of them assertions
nothing in this project checks. Two were regressions of rules this
repo had already settled rather than ordinary staleness, and both are
now written down in AGENTS.md so the next draft does not repeat them.

The two regressions:

  * `upgradable/0` said the record is synthesised when the system
    started without a `releases/RELEASES` file "it could read". That is
    the *readable* phrasing five successive corrections to the refusal
    message had already discarded - it misses malformed terms - and it
    named the file unqualified, which castle#23 forbids because RELDIR
    and `{sasl, releases_dir}` relocate it. The @doc now says what the
    refusal says: the file `:release_handler` accepts, no one property
    of the file being the test, and the path qualified.

  * `commit/1` said committing an already-permanent version "succeeds
    and changes nothing". That was true before castle#14 moved
    materialisation inside `commit`'s serialised region.
    `make_permanent/1` is still a no-op for such a version, but the
    target's `sys.config` has been rewritten from current provider
    inputs by then, and a provider that fails now fails the command.

The five others:

  * `report!/1` does not make every failure a `Castle.Error`.
    `installed/5` settles the marker and re-raises what
    `install_release/1` raised, threw or exited with, so automation
    rescuing only `Castle.Error` would read a blown-up command as a
    success. The moduledoc now says which failures become which.

  * The install preflight is documented in the order it happens: the
    ERTS guard, the release-record lookup and its refusal, the
    pending-marker refusal, materialisation, arming - five steps, before
    `install_release/1` rather than before any `:release_handler` call,
    since the lookup is one. The pending-marker refusal was missing
    entirely, and it is what stops a refused caller replacing the
    configuration a pending install is about to boot.

  * "Anything that takes the system down brings the previous version
    back" is false for a restart install. The launcher consumes the
    marker pair on the next start and boots the target; it is the
    restarts after that one which return to the previous permanent
    version. RELEASE.md said the same thing and is corrected with it.

  * `bin/castle commit` with no version selects a `:current` release
    and exits non-zero when there is none - not "the version running
    now", which a system on its permanent release also has.

  * `unpacked` is staged and not currently installed, a version a
    rollback returned to that state included. `running/1`'s own @doc
    already said so.

And the specs: two @specs on a five-arity defaulted definition left
`install/2..4` unspecced, contradicting this branch's own claim that
the whole public surface carries one. All five are now present, checked
with `Code.Typespec.fetch_specs(Castle)` against
`Castle.__info__(:functions)` rather than by reading the source - ExDoc
renders only the widest arity's spec, so the page could not have shown
the gap.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN
@ausimian

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c0f7c073a5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/castle.ex Outdated
Comment thread AGENTS.md Outdated
Two claims a review caught, both the same shape as the seven before them:
a qualification applied in one place and not the neighbouring one.

unpack/1 said the tarball has to be copied into the deployment's releases
directory. release_handler reads whichever directory RELDIR or the sasl
releases_dir parameter names - do_unpack_release joins RelDir, and RelDir is
that override when it is set - so an operator following it on such a
deployment copies the archive somewhere unpack will not look. The
release-record file already carried this caveat two @docs away.
customize/1's :tar warning repeated the same unqualified path and now points
at unpack/1 for it.

And AGENTS.md still said a failure raises Castle.Error three paragraphs after
the rule correcting exactly that. installed/5 re-raises an exception, throw or
exit out of install_release/1 unchanged, so a rescue narrowed to Castle.Error
misses a command that blew up rather than refused. Said as refusal rather than
failure, with a paragraph on why the blanket version keeps coming back. The
module comment on the boundary had it too.

Co-Authored-By: Claude <noreply@anthropic.com>
@ausimian

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9e152efc4f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/castle.ex Outdated
remove/1's doc said the command deletes erts-<vsn>, using the same vsn as its
release-version argument. do_remove_release/4 reads erts_vsn out of the
release record and removes erts-<EVsn> only when no remaining release refers
to that emulator - and the two numbers are unrelated, normally different ones:
a release at 0.1.1 may be carrying erts-16.2. So the doc named a directory
that generally does not exist.

The same placeholder was reused in five other places where it sits beside
lib/<app>-<vsn> and so reads as the same version, including the ERTS guard's
own refusal message, which ships. All now say erts-<erts_vsn>. The synthetic
release fixture's erts-<vsn>/bin is left alone: there is no adjacent
lib/<app>-<vsn> and the version genuinely is the emulator's.

The whole-message assertion in erts_guard_test failed on the reword, which is
what it is for - the message has been wrong three times about what it can
claim, so changing it deliberately means editing that test deliberately.

Co-Authored-By: Claude <noreply@anthropic.com>
@ausimian

Copy link
Copy Markdown
Owner Author

@codex review

AGENTS.md already quoted do_remove_release/4 deleting filename:join(Root,
"erts-" ++ EVsn) correctly; the drift was in the placeholder prose around it,
which reused <vsn> for both namespaces and so named a directory that generally
does not exist. Note it at the authority, with where it had already got to -
remove/1's @doc and the ERTS guard's refusal message, which ships.

Co-Authored-By: Claude <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e8bba5a049

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/castle.ex Outdated
Comment thread lib/castle.ex Outdated
…laim

Two claims a review found, both asserting more than the code does.

The unpack doc offered RELDIR and the sasl releases_dir parameter as ways to
name another release directory. rel_dir/0 is Path.join(Deployment.root_dir(),
"releases") and consults neither, so setting one does not merely go unread: it
moves the directory the handler unpacks into and keeps its records in, while
the ERTS guard reading the target .rel (commands.ex:128) and the step writing
the target configuration (commands.ex:743) both still resolve
<root>/releases/<vsn>. The doc was directing operators into an upgrade that
cannot complete. It now says so. AGENTS.md already had this right and cites
issue #23; the omission was only in the public doc.

The moduledoc said the commands that are questions "succeed with nothing to
say at all". releases/0 is a question and prints a line per release through
report!/1, which its own @doc promises, so automation was given the wrong
stdout contract. What gets printed varies by command, not by kind:
upgradable/0 and running/1 return {:ok, []}, releases/0 returns the table.

Co-Authored-By: Claude <noreply@anthropic.com>
@ausimian

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 131ef59217

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread AGENTS.md Outdated
The passage explaining what is published and what is hidden named two
exceptions and then said every other function is a command an operator
invokes. running/1 is not: bin/castle dispatches exactly releases,
upgradable, unpack, install, remove and commit, and running/1 has no
subcommand - its only shipped caller is the confirmation loop inside
bin/castle install. Its own new @doc says so, so AGENTS.md contradicted the
file it is describing.

It stays published, but for a different reason than the other two: it answers
a read-only question and mutates nothing, and automation driving rpc rather
than bin/castle needs it to know when an install has finished booting. That is
the distinction from make_releases/0, which mutates bookkeeping once and is
useless by hand.

Co-Authored-By: Claude <noreply@anthropic.com>
@ausimian

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fec90e98fa

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/castle.ex Outdated
The previous commit overcorrected. Fixing a paragraph that offered RELDIR and
the sasl releases_dir parameter as supported, it also asserted that
<root>/releases is the only release directory Castle resolves - which is wrong
about the tarball, the one thing the paragraph exists to locate. unpack/1 hands
the name straight to :release_handler.unpack_release/1 and joins nothing;
do_unpack_release/4 does Tar = filename:join(RelDir, ReleaseName ++ ".tar.gz")
against the handler own releases directory, which the overrides move. An
operator following the text would have failed at the unpack step.

The text now separates the two halves: the tarball goes where the handler
reads it, and the overrides move that; everything Castle resolves for itself
is joined onto code:root_dir(), so an override still leaves the halves
disagreeing and an unpack that succeeds is followed by an install that cannot.
The unsupported warning and the #23 reference stay.

Also corrects a claim the previous commit made in passing, in its message and
its prose: commands.ex:128 is in create_releases/3, on the make_releases/0
path, not in the ERTS guard. The install-side consumers of Castle rel_dir are
restart_planned?/3, unclaimed/3, materialise/3, arm/4 and installed/5.

Co-Authored-By: Claude <noreply@anthropic.com>
@ausimian

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: edadd5411f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@ausimian
ausimian merged commit bc19bf5 into release/1.0.0 Aug 23, 2026
7 checks passed
@ausimian
ausimian deleted the issue/11-docs branch August 23, 2026 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant